Skip to content

ci: run update-changelogs on comment for non-release PRs - #10077

Merged
cryptodev-2s merged 2 commits into
mainfrom
ci/update-changelogs-comment-non-release
Sep 2, 2026
Merged

ci: run update-changelogs on comment for non-release PRs#10077
cryptodev-2s merged 2 commits into
mainfrom
ci/update-changelogs-comment-non-release

Conversation

@cryptodev-2s

@cryptodev-2s cryptodev-2s commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Explanation

@metamaskbot update-changelogs never worked outside release PRs. #8443 documented it as working on any non fork PR, but shipped both comment jobs gated on is-release == 'true', so commenting on a normal PR did nothing. This makes the comment trigger actually work.

Job Before After Why
react-to-comment is-release == 'true' && issue_comment issue_comment react to every command comment, not just ones on release PRs
update-changelogs is-release == 'true' is-release == 'true' || issue_comment run on demand on any PR, while release PRs keep running automatically

Release PR behavior is unchanged. Opening a non release PR still triggers nothing automatically, you have to ask for it. Fork PRs are still skipped.

Also documented the command in docs/processes/updating-changelogs.md, since it was undiscoverable.

References

Related to #8443

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Low Risk
CI workflow condition changes and documentation only; no runtime application or security-sensitive logic.

Overview
Fixes @metamaskbot update-changelogs so it works on ordinary PRs, not only release PRs. Previously both comment-related jobs required is-release == 'true', so commenting on a non-release PR did nothing despite earlier docs implying it should work.

react-to-comment now runs on any issue_comment (drops the release check) so the bot can 👍 every command comment. update-changelogs runs when the PR is a release or when triggered via issue_comment, preserving automatic updates on release PRs while enabling on-demand runs elsewhere. Fork PRs remain unsupported per existing behavior.

Adds a Generating changelog entries for dependency bumps section in docs/processes/updating-changelogs.md describing the comment flow, timing, and limitations.

Reviewed by Cursor Bugbot for commit d1d8705. Bugbot is set up for automated code reviews on this repo. Configure here.

@cryptodev-2s cryptodev-2s self-assigned this Sep 2, 2026
@cryptodev-2s
cryptodev-2s force-pushed the ci/update-changelogs-comment-non-release branch from f17f696 to 17aea1a Compare September 2, 2026 14:04
@cryptodev-2s
cryptodev-2s marked this pull request as ready for review September 2, 2026 14:05
@cryptodev-2s
cryptodev-2s requested a review from a team as a code owner September 2, 2026 14:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 17aea1a. Configure here.

name: Update changelogs
needs: is-release
if: ${{ needs.is-release.outputs.is-release == 'true' }}
if: needs.is-release.outputs.is-release == 'true' || github.event_name == 'issue_comment'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing commenter write-access gate

High Severity

Commenting @metamaskbot update-changelogs now reaches update-changelogs, which mints a write token and pushes to the PR branch, but is-fork still accepts any commenter. On this public repository, anyone can start privileged writes on a non-fork PR.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 17aea1a. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre existing

@FrederikBolding FrederikBolding Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems correct, we would need to bail out on forks. Which we aren't doing anymore if we allow it on non-releases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is pre-existing behavior. It means that on any PR opened from within the repository, a comment from any person could trigger the workflow. Nothing changes here.

The existing fork check only verifies that the workflow is not running from a forked repository. It does not check whether the person commenting on a non-fork PR is a legitimate/authorized user.

@cryptodev-2s cryptodev-2s Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean even previously when it was scoped to release PRs only. Anyone could have triggered this by a comment.

On the bot's actual point, it's about who typed the comment, not about forks.

@cursor
cursor Bot requested review from Mrtenz and mcmire September 2, 2026 14:11

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note: Should we consider running this on draft releases too? It seems a bit confusing you have to bring it out of draft to get the bot to run automatically

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only do this for non-draft PRs. We assume CI could still be failing, or the person may not have prepared the release correctly or fixed all the constraints (for using the correct versions), so the workflow needs to be safe to run more than once. (assuming people are correctly using the draft state)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I can see both sides on this and don't have a strong opinion. That said even if more people are likely to post a comment than wait for the workflow to run then it would act similar to the "update LavaMoat policies" workflow in Extension, which is an established pattern. So maybe the way you have it is okay?

name: Update changelogs
needs: is-release
if: ${{ needs.is-release.outputs.is-release == 'true' }}
if: needs.is-release.outputs.is-release == 'true' || github.event_name == 'issue_comment'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow update-changelogs to run automatically on non-release PRs too? Let's say someone bumps a dependency in a feature branch. If we are encouraging engineers to update the changelog in advance instead of within release PRs, then it seems that we would want the dependency to be logged in the feature branch and not skipped.

@cryptodev-2s cryptodev-2s Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that too. However, doing this would cause many runs to consume compute credits unnecessarily, since the likelihood of a PR actually needing changelog updates for package bumps is relatively low.

We’re still planning to automatically plug this workflow into Dependabot PRs, for example. So I believe keeping it this way is still the better option.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, alright.

Comment thread docs/processes/updating-changelogs.md Outdated
- Split disparate changes from the same pull request into multiple entries if necessary.
- Omit reverted changes from the changelog.

## Updating changelogs automatically

@mcmire mcmire Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To mention what exactly is being updated in changelogs, I wonder if this should be:

Suggested change
## Updating changelogs automatically
## Generating changelog entries for dependency bumps

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied here d1d8705

Comment thread docs/processes/updating-changelogs.md Outdated
A few things to know:

- This works on any pull request, not just release pull requests. Release pull requests also get this automatically when they are opened.
- If the action pushes a commit, remember to pull it before you push again.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
- If the action pushes a commit, remember to pull it before you push again.
- If the action pushes a commit to your branch, remember to pull it locally before you push again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied here d1d8705

Comment thread docs/processes/updating-changelogs.md Outdated

## Updating changelogs automatically

Some changelog entries are mechanical, such as the entries that record dependency bumps across packages. You can get these written for you instead of writing them by hand:

@mcmire mcmire Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if "mechanical" is the right word here. Maybe we want:

Suggested change
Some changelog entries are mechanical, such as the entries that record dependency bumps across packages. You can get these written for you instead of writing them by hand:
Recording dependency bumps within package changelogs can be tedious. You can have these entries generated for you instead of writing them by hand:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied here d1d8705

@cryptodev-2s
cryptodev-2s requested a review from mcmire September 2, 2026 18:29

@mcmire mcmire left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@cryptodev-2s
cryptodev-2s added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit 5adbf04 Sep 2, 2026
436 checks passed
@cryptodev-2s
cryptodev-2s deleted the ci/update-changelogs-comment-non-release branch September 2, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants